Windows support #42

mjzk · 2024-09-12T12:06:47Z

fixs #41

CLAassistant · 2024-09-12T12:19:43Z

All committers have signed the CLA.

weekday-grandine-io · 2024-10-09T04:06:51Z

.github/workflows/ci.yml

-    # There should be no need to install Rustup or a Rust toolchain explicitly.
-    # The `ubuntu-22.04` image already includes Rustup:
-    # <https://github.com/actions/runner-images/blob/ubuntu22/20240401.4/images/ubuntu/Ubuntu2204-Readme.md#rust-tools>
-    # Running `rustc` or Cargo should automatically install the toolchain specified in `rust-toolchain.toml`.


suggestion: Instead of removing the comment, update it to explain the same is true for windows-2022.

# The `ubuntu-22.04` and `windows-2022` images already include Rustup: # - <https://github.com/actions/runner-images/blob/ubuntu22/20240401.4/images/ubuntu/Ubuntu2204-Readme.md#rust-tools> # - <https://github.com/actions/runner-images/blob/ubuntu22/20240401.4/images/windows/Windows2022-Readme.md#rust-tools>

OK. My personal think for the removal here is: These comments are of little significance. ubuntu-latest is not ubuntu-22.04 mentioned here but Ubuntu 24.04. It doesn't make much sense to let developers maintain these changes.

But, I can also accept that if you, as a committer, think it is necessary to keep. So I will modify it according to your suggestion.

weekday-grandine-io · 2024-10-09T04:15:40Z

.github/workflows/ci.yml

+      shell: bash
+      run: bash scripts/ci/clippy.bash --deny warnings


polish: Specifying Bash as the interpreter explicitly should not be necessary.

This step will only run on Linux.
It has worked on Linux until now.

I assume the addition of bash is an oversight.

Suggested change

shell: bash

run: bash scripts/ci/clippy.bash --deny warnings

run: scripts/ci/clippy.bash --deny warnings

OK. The addition of bash is intentional. The two run command diff here is: the original requires the script scripts/ci/clippy.bash in the executable mode. This first has security concerns. But most problem here is, if you check out the script in unsupport OSes and FSes like in Windows, the executable mode will be lost. Then, although you want to execute it in WSL like things. It does not work.

But, I can also accept that if you, as a committer, think you want to use the original command. So I will modify it according to your suggestion.

weekday-grandine-io · 2024-10-09T04:16:39Z

Cargo.toml

-unsafe_code = 'forbid'
+unsafe_code = 'deny'


issue: I am opposed to the use of unsafe in this workspace.

Try using winsafe.
winsafe does not have a wrapper for GetProcessMemoryInfo, but you could fork it and even submit a pull request upstream.

Also, it makes little sense to set a lint like unsafe_code to deny.
Unsafe Rust is already opt-in via the unsafe keyword.

@weekday-grandine-io Basically, I do not agree with you on this point.

The sense of deny is to make developer have the chance to opt-out forced lint errors. Here, the result of the behavior that I override that unsafe lint error is controllable and has minimal impact. To forbid unsafe in a large Rust engineering, like Grandine, is just a cover-up in the current situation where we heavily depends on a large number of unsafe libraries.

Although firing a pull request upstream is possible, it is time-consuming, and no guarantee of upstream willing to accept new additions or modifications. I think, that's why we have so many forks in our grandinetech org.

Although I can not agree with you on this point, I can also accept that if you, as a committer, think you just want to use unsafe_code = 'forbid'. But I suggest to reconsidering the fixing way.

There are three kinds of actions now:

keep this change in PR and nothing need to be done.

firstly to fork a project in our grandinetech org, to quick apply update in current PR. Then, make Grandine depends on it and go on...

firstly to fork a project anywhere, to submit an PR upstream. Then, to wait it accepted and make Grandine depends on it after that and go on...

@weekday-grandine-io @sauliusgrigaitis What do you think?

So in the core team, we think that unsafe code is really not desired in Grandine codebase (dependencies is another story). So in order to ensure this philosophy we use unsafe_code = 'forbid'. For sure it's not very convenient whenever a tricky situation comes, but we try to be creative to workaround particular cases so we can still proceed using unsafe_code = 'forbid'.

@sauliusgrigaitis OK.

I understand our decision although I think it is not very meaningful now. For example, our bad stackoverflow in the debug mod currently. The philosophy of deny, in fact, also forbids the unsafe by default. There is no unintentional unsafe. But again, I respect the core team's decision.

I will follow the above action#3 which was suggested by @weekday-grandine-io , although you didn't suggest which is better in 2 and 3.

Apparently this causes an issue that this PR is being locked by upstream. I will research and contact upstream as soon as possible to resolve this issue. However, there may not be a clear timeline.

weekday-grandine-io · 2024-10-09T04:24:02Z

Cargo.toml

+windows = { version = "0.58", features = [
+    "Win32_Foundation",
+    "Win32_System_ProcessStatus",
+    "Win32_System_Threading",
+    "Win32_System_SystemInformation",
+] }


nitpick: We have a convention of using literal strings in TOML files whenever possible.

Also, we usually order features alphabeticaly and do not wrap them over multiple lines.

Suggested change

windows = { version = "0.58", features = [

"Win32_Foundation",

"Win32_System_ProcessStatus",

"Win32_System_Threading",

"Win32_System_SystemInformation",

] }

windows = { version = '0.58', features = [

'Win32_Foundation',

'Win32_System_ProcessStatus',

'Win32_System_SystemInformation',

'Win32_System_Threading',

] }

or

Suggested change

windows = { version = "0.58", features = [

"Win32_Foundation",

"Win32_System_ProcessStatus",

"Win32_System_Threading",

"Win32_System_SystemInformation",

] }

windows = { version = '0.58', features = ['Win32_Foundation', 'Win32_System_ProcessStatus', 'Win32_System_SystemInformation', 'Win32_System_Threading'] }

weekday-grandine-io · 2024-10-09T04:26:09Z

ad_hoc_bench/src/main.rs

-    jemalloc_ctl::epoch::advance().map_err(Error::msg)?;
+    jemalloc_ctl::epoch::advance().map_err(anyhow::Error::msg)?;


nitpick: Import types at the top of the file.

#[cfg(target_os = "windows")] use ::{anyhow::Error, bytesize::ByteSize, jemalloc_ctl::Result as JemallocResult};

This import is just used for not(target_os = "windows"). To prompt it to the top in import will cause additional verbose imports.

But, I can also accept that if you, as a committer, think you want to use this more verbose imports.

weekday-grandine-io · 2024-10-09T05:17:07Z

metrics/src/metric_sys.rs

+        .is_ok()
+    } {
+        memory_process_bytes = mem_counters.WorkingSetSize as _;
+        trace!("Memory usage: {} bytes", memory_process_bytes);


nitpick: Most error and log messages in the codebase start in lowercase.

weekday-grandine-io · 2024-10-09T05:25:23Z

metrics/src/metric_sys.rs

+
+#[inline(always)]
+#[cfg(target_os = "windows")]
+fn filetime_to_seconds(ft: &FILETIME) -> u64 {


polish: It would be more idiomatic to pass FILETIME as an owned value.

FILETIME is a small struct that implements Copy.

Suggested change

fn filetime_to_seconds(ft: &FILETIME) -> u64 {

fn filetime_to_seconds(ft: FILETIME) -> u64 {

accepted.

(But passing by ref/pointer indeed follows the FFI convention in the Rust community.)

weekday-grandine-io · 2024-10-09T05:32:16Z

metrics/src/metric_sys.rs

+fn filetime_to_seconds(ft: &FILETIME) -> u64 {
+    let total_seconds = ((ft.dwHighDateTime as u64) << 32 | ft.dwLowDateTime as u64) / 10_000_000;
+    total_seconds
+}


thought: It would be more idiomatic to return Duration, but that's probably out of scope.

Representing the number of seconds with u64 limits precision to 1 second, but that seems to be required by the Client Stats Proposal.

It is interesting to see that we have a specification for client stats. This method is indeed a quick method to imitate the original Linux side logic. We can change it in the future if necessary.

weekday-grandine-io · 2024-10-09T05:39:13Z

metrics/src/metric_sys.rs

+#[inline(always)]
+#[cfg(target_os = "windows")]
+fn filetime_to_seconds(ft: &FILETIME) -> u64 {
+    let total_seconds = ((ft.dwHighDateTime as u64) << 32 | ft.dwLowDateTime as u64) / 10_000_000;


suggestion: Document the magic number somehow.

Consider extracting a constant with a self-documenting name.
Consider linking to the documentation of FILETIME.

weekday-grandine-io · 2024-10-09T05:48:25Z

metrics/src/metric_sys.rs

+#[inline(always)]
+#[cfg(target_os = "windows")]
+fn filetime_to_seconds(ft: &FILETIME) -> u64 {
+    let total_seconds = ((ft.dwHighDateTime as u64) << 32 | ft.dwLowDateTime as u64) / 10_000_000;


note: FILETIME is a kind of timestamp (with an unusual epoch), but GetSystemTimes uses it to represent a duration.

mjzk · 2024-10-09T11:53:17Z

@weekday-grandine-io thanks for the detailed review. I will react according to my understanding. Although I don't think some of your opinions are necessarily valid, I will give priority to your suggestions.

mjzk · 2024-11-06T11:03:38Z

@sauliusgrigaitis @weekday-grandine-io

For syncing PR to develop branch easier, I do a squash on all commits. And finally CI passed again.

After contacting with upstream, the relevant Windows stats successfully rely on winsafe. This also greatly simplifies the implementation of the metric_sys mod.

Other changes are also made according to the review comments even with some of the changes which I not fully agree with.

If you need any other changes, please let me know and I will try to complete them even after the end of EPF cohort 5.

mjzk force-pushed the windows_support branch 5 times, most recently from 06fc648 to 6b2df75 Compare September 25, 2024 23:58

mjzk marked this pull request as ready for review September 26, 2024 01:13

mjzk mentioned this pull request Sep 26, 2024

Windows support #41

Open

5 tasks

weekday-grandine-io suggested changes Oct 9, 2024

View reviewed changes

mjzk force-pushed the windows_support branch 2 times, most recently from f219b9b to 4b987ff Compare November 6, 2024 06:55

feat: support Windows

b14ea8a

mjzk force-pushed the windows_support branch from 4b987ff to b14ea8a Compare November 6, 2024 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows support #42

Windows support #42

mjzk commented Sep 12, 2024

CLAassistant commented Sep 12, 2024 •

edited

Loading

weekday-grandine-io Oct 9, 2024

mjzk Oct 10, 2024

weekday-grandine-io Oct 9, 2024

mjzk Oct 10, 2024

weekday-grandine-io Oct 9, 2024

mjzk Oct 11, 2024

sauliusgrigaitis Oct 11, 2024

mjzk Oct 12, 2024 •

edited

Loading

weekday-grandine-io Oct 9, 2024

mjzk Oct 11, 2024

weekday-grandine-io Oct 9, 2024

mjzk Oct 11, 2024

weekday-grandine-io Oct 9, 2024

mjzk Nov 4, 2024

weekday-grandine-io Oct 9, 2024

mjzk Nov 4, 2024 •

edited

Loading

weekday-grandine-io Oct 9, 2024

mjzk Nov 4, 2024 •

edited

Loading

weekday-grandine-io Oct 9, 2024

mjzk Nov 4, 2024

weekday-grandine-io Oct 9, 2024

mjzk Nov 4, 2024

mjzk commented Oct 9, 2024

mjzk commented Nov 6, 2024 •

edited

Loading

	shell: bash
	run: bash scripts/ci/clippy.bash --deny warnings
	run: scripts/ci/clippy.bash --deny warnings

		jemalloc_ctl::epoch::advance().map_err(Error::msg)?;
		jemalloc_ctl::epoch::advance().map_err(anyhow::Error::msg)?;

	fn filetime_to_seconds(ft: &FILETIME) -> u64 {
	fn filetime_to_seconds(ft: FILETIME) -> u64 {

		unsafe_code = 'forbid'
		unsafe_code = 'deny'

Windows support #42

Are you sure you want to change the base?

Windows support #42

Conversation

mjzk commented Sep 12, 2024

CLAassistant commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjzk Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjzk Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjzk Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjzk commented Oct 9, 2024

mjzk commented Nov 6, 2024 • edited Loading

CLAassistant commented Sep 12, 2024 •

edited

Loading

mjzk Oct 12, 2024 •

edited

Loading

mjzk Nov 4, 2024 •

edited

Loading

mjzk Nov 4, 2024 •

edited

Loading

mjzk commented Nov 6, 2024 •

edited

Loading